fix(vm_service): state machine does not run reconcileMachineAddresses#721
fix(vm_service): state machine does not run reconcileMachineAddresses#721
Conversation
|
I've added a CloneVM mock to the testcase, now it's rather complete. The only thing that could be added would be a full bootstrap.go test, but that's a full can of worms I'll leave to somebody else to implement. Now we know that you need to requeue at least 4 times to deploy a capmox machine. |
65278
force-pushed
the
710_fix_proxmoxmachine_status_addresses
branch
from
2cea172 to
ade1f57
Compare
65278
force-pushed
the
710_fix_proxmoxmachine_status_addresses
branch
from
ade1f57 to
e137c7c
Compare
65278
force-pushed
the
710_fix_proxmoxmachine_status_addresses
branch
from
3302866 to
f9c2804
Compare
65278
force-pushed
the
710_fix_proxmoxmachine_status_addresses
branch
from
f9c2804 to
2e137c6
Compare
|
Actual deployment performed. It turns out we had problems way deeper than the initial problem: A qmstart task always fails on our proxmox. The VM starts anyway with two different errors. I've made it so that taskfailure now knows about qmstart and treats it as a special case. Because this triggered the quality gate, I've had claude add a test for task.go, even though we're only testing invariants and the mock kubernetes client. |
65278
force-pushed
the
710_fix_proxmoxmachine_status_addresses
branch
from
2e137c6 to
22fd914
Compare
mcbenjemaa
left a comment
mcbenjemaa
left a comment
There was a problem hiding this comment.
Overall looks good, just some remarks,
The test added in vm_test is great.
You never know, you know. It's not a bad thing to assert invariants. |
The state machine was set to the wrong state after reconcilePowerState, therefore proxmoxmachine.Status.Addresses was never populated. Our tests for reconcileMachineAddresses succesfully ran as the code itself was fine. Our E2E tests did not fail because we do not depend on this field. To catch these kinds of errors in the future, I've made a new test for ReconcileVM which tries to test its workings in their entirety. fix: 710 fix: 714
Exercise task lifecycle via ReconcileInFlightTask, disk resize via ResizeDisk, and fix a misleading comment. Co-Authored-By: Claude <noreply@anthropic.com>
TaskFailure State is now only set if the action is not qmstart. It looks impossible to detect a failure in qmstart. This fixes the state machine transition in ReconcileVM.
Coverage for task.go dropped below the gate after the qmstart fix. These tests exercise every switch branch but are mostly tautological since the logic is straightforward and already covered by the end-to-end ReconcileVM test. Co-Authored-By: Claude <noreply@anthropic.com>
wikkyk
force-pushed
the
710_fix_proxmoxmachine_status_addresses
branch
from
6f06faf to
75dc717
Compare
|
Issue #, if available:
#710
#714
Description of changes:
The state machine was set to the wrong state after reconcilePowerState, therefore proxmoxmachine.Status.Addresses was never populated. Our tests for reconcileMachineAddresses succesfully ran as the code itself was fine. Our E2E tests did not fail because we do not depend on this field.
To catch these kinds of errors in the future, I've made a new test for ReconcileVM which tries to test its workings in their entirety.
Testing performed:
make test